Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add retry_if_exception_message #127

Merged
merged 1 commit into from
Jul 20, 2018
Merged

Add retry_if_exception_message #127

merged 1 commit into from
Jul 20, 2018

Conversation

Brian-Williams
Copy link
Contributor

@Brian-Williams Brian-Williams commented Jul 16, 2018

From issue #125.

Add retry classes for exception messages.

I'm very open to name suggestions. Specifically wondering if retry_if_not_exception_message or retry_until_exception_message is best.

retry_if_exception_message and it's complement act different out of necessity. The normal class can end if there is no exception whereas it's inverse needs to retry if there is no exception message.

It's worth noting in my branch unit tests are intermittently failing on wait class backwards incompatibility. I don't believe this is caused by any of my changes, but it's worth noting.

@Brian-Williams Brian-Williams force-pushed the master branch 2 times, most recently from b226357 to 3c7e03c Compare July 18, 2018 07:39
@Brian-Williams Brian-Williams changed the title WIP: Add retry_if_exception_message Add retry_if_exception_message Jul 18, 2018
"{}() missing 1 required argument 'message' or 'match'".format(
self.__class__.__name__))

def _no_failure(self):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staticmethod

Not sure it's worth having a method, just an attribute would be enough.


return inverted_predicate

def _no_failure(self):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staticmethod

Not sure it's worth having a method, just an attribute would be enough.

"{}() takes either 'message' or 'match', not both".format(
self.__class__.__name__))
self.predicate = self._predicate_factory(message, match)
self.require_exception = require_exception
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not seem to be related to the heart of the function. I'd remove that, and put that in a different class (and PR) if that's needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a shortcut that may be too specific to my use case. I'm happy to remove it and simply inherit from this in my projects.

@tenacity.retry(retry=tenacity.retry_if_exception_message("My message")
def my_fnc():
    try:
        # sometimes raise MyException("My message")
     except Exception as e:
        raise e
    else:
        raise ShouldveBeenException("We expected an exception to check message of, but didn't receive one")

whereas with the optional input it can be shorter and also not risk having the no-exception exception message matching the retry predicate.

@tenacity.retry(retry=tenacity.retry_if_exception_message("My message", require_exception=True))
def my_fnc():
    # sometimes raise MyException("My message")

I think I was too focused on my negative testing use case though. Happy to remove as it simplifies the classes.

@@ -111,6 +112,61 @@ def __call__(self, attempt):
return not self.predicate(attempt.result())


class retry_if_exception_message(retry_base):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should inherit from retry_if_exception

"""Retry logic for no failure."""
return False

def __call__(self, attempt):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed if you inherit from retry_if_exception

raise TypeError(
"{}() takes either 'message' or 'match', not both".format(
self.__class__.__name__))
self.predicate = self._predicate_factory(message, match)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just pass the result to the __init__ method of retry_if_exception here (since you will inherit from it). The factory method is not even really needed IMHO, you can put that code in __init__.


class retry_if_not_exception_message(retry_if_exception_message):
"""Retries until an exception message equals or matches."""

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just set invert the predicate in __init__ with a lambda I'd say.

@Brian-Williams
Copy link
Contributor Author

Thanks for the review!

Removed require_exception, which allowed us to inherit from retry_if_exception as suggested.

Removed _no_failure fnc as it no longer has a use case if retry_if_not_exception_message is the only __call__ in this PR; retry_if_exception_message can simply inherit that magic method.

Fixed a bug in the assumption of the errormessage retrieval and wrote an additional unit test to handle the positive case of retry_if_not_exception_message.

Rebased on jd/master to a single commit.

The _predicate_factory was originally wrapped.

Wrapping made it more confusing. Easier to just have it
clearly be the inverted predicate.

Considered using an anti-pattern to set a dynamic name, but in reality
the method that is decorated should make it obvious if it's initialized
with message or match.

Add imports to init
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants